Skip to content

doc: add large pull requests contributing guide#62829

Open
mcollina wants to merge 1 commit intonodejs:mainfrom
mcollina:doc/large-pull-requests
Open

doc: add large pull requests contributing guide#62829
mcollina wants to merge 1 commit intonodejs:mainfrom
mcollina:doc/large-pull-requests

Conversation

@mcollina
Copy link
Copy Markdown
Member

@mcollina mcollina commented Apr 19, 2026

Fixes: #62752

Adds a contributing guide for large pull requests (3000+ lines), covering:

  • Threshold definition and applicability
  • Restriction to collaborators only
  • Design document and review guide requirements
  • Two TSC approvals (same as semver-major)
  • Dependency change separation
  • Feature fork workflow in separate repositories
  • Reviewer guidance

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Apr 19, 2026

Review requested:

  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 19, 2026
panva
panva reviewed Apr 19, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@panva panva left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • We should link to it from CONTRIBUTING.md, pull-requests.md, collaborator-guide.md (or some subset of those)
  • We should exclude routine dependency/wpt/test bot-issued PRs, this can either be excluded explicitly with additional rules or by changing the classification put forth by the Overview or What qualifies as a large pull request sections
  • dependency changes in a separate commit is at odds with having each commit self-contained, this is fine when the commits get squashed but the first commit's message is used for the squash so the first commit would be the non-deps changes? seems odd and counter-intuitive
  • Design document - I 100% discourage linking outside documents, a detailed PR description should also suffice, 3000 can easily be hit with just thorough test coverage for an otherwise trivial change, how would a design document help such PRs?
  • Both Separating test additions from implementation changes. and Splitting documentation into its own pull request. strategies to avoiding large PRs are contrary to what we usually request of contributions. Those strategies only work when the contribution isn't "done" or "shipped" and is excluded from builds.

@mcollina mcollina force-pushed the doc/large-pull-requests branch from e9d9a4c to 7552c50 Compare April 20, 2026 10:28
RafaelGSS
RafaelGSS approved these changes Apr 20, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it. LGTM

Comment thread doc/contributing/large-pull-requests.md
@joyeecheung
Copy link
Copy Markdown
Member

joyeecheung commented Apr 22, 2026

The specification here doesn't seem very clear:

  1. What are 3000 line of changes: deletion and addition combined? Or just one of them? Splitting a flaky test monolith to avoid hiding regressions for example can easily pass the threshold even though there's not really an interesting architecture to explain and 2 TSC review seems like overkill
  2. What happens when a PR initially isn't big but grows to be bigger as more tests/docs are added?

@mcollina
Copy link
Copy Markdown
Member Author

mcollina commented Apr 22, 2026

The specification here doesn't seem very clear:

  1. What are 3000 line of changes: deletion and addition combined? Or just one of them? Splitting a flaky test monolith to avoid hiding regressions for example can easily pass the threshold even though there's not really an interesting architecture to explain and 2 TSC review seems like overkill

We have to draw a line, and this is as good as any. I proposed 5k but I was asked to lower it.
We need this to be "easy" to check from the GitHub interface.

I would be happier with a 3k of non-test/non-doc changes, but that would require better tooling, which I'd prefer to avoid creating now.

What would work for you?

  1. What happens when a PR initially isn't big but grows to be bigger as more tests/docs are added?

The number that matters is at landing time.

@ShogunPanda
Copy link
Copy Markdown
Contributor

ShogunPanda commented Apr 22, 2026

The specification here doesn't seem very clear:

  1. What are 3000 line of changes: deletion and addition combined? Or just one of them? Splitting a flaky test monolith to avoid hiding regressions for example can easily pass the threshold even though there's not really an interesting architecture to explain and 2 TSC review seems like overkill

In the document it says: A pull request is considered large when it exceeds **3000 lines** of combined additions and deletions..

So ADD + (- DEL) > 3000

  1. What happens when a PR initially isn't big but grows to be bigger as more tests/docs are added?

I would say as soon as it reaches the threshold the new rules apply. This also means that if with further commits the PR shrinks, rules do not apply anymore.
I guess the commit-queue bot must have a rule to ultimately enforce this.

@jasnell
Copy link
Copy Markdown
Member

jasnell commented Apr 22, 2026

FYI, I've added the new large-pr label that we can use to explicitly marge large PRs that would fall under this policy

@jasnell
Copy link
Copy Markdown
Member

jasnell commented Apr 22, 2026

ADD + ( -DEL) > 3000 is more complicated than I think it needs to be. I'd prefer just to look at additions but happy to go with consensus if others agree with the proposed alternative.

@jasnell
Copy link
Copy Markdown
Member

jasnell commented Apr 22, 2026

@nodejs/tsc ... we could use some more TSC scrutiny on this policy change.

UlisesGascon
UlisesGascon reviewed Apr 22, 2026
View reviewed changes
Comment thread doc/contributing/large-pull-requests.md
* Highlight the most critical sections that need careful attention.
* Include a testing plan explaining how the change has been validated and
how reviewers can verify the behavior.

Copy link
Copy Markdown
Member

@UlisesGascon UlisesGascon Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Organize commits in a logical sequence so that reviewers can follow the progression of the change incrementally, rather than reviewing the entire diff at once.

Organize commits in a logical sequence that tells a clear story of the change, allowing reviewers to follow the progression incrementally. Ideally we can follow the author's train of thought step by step rather than parsing a monolithic diff 🤔

Copy link
Copy Markdown
Member Author

@mcollina mcollina Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in direct conflict with what has been asked by @joyeecheung and others.

(This is how I operate, btw, creating a PR in small increments).

@mcollina mcollina force-pushed the doc/large-pull-requests branch from 7552c50 to 57506d2 Compare April 23, 2026 08:46
@mcollina mcollina force-pushed the doc/large-pull-requests branch from 57506d2 to 104bc61 Compare April 23, 2026 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@panva panva panva left review comments

@UlisesGascon UlisesGascon UlisesGascon left review comments

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@jasnell jasnell jasnell approved these changes

@anonrig anonrig anonrig approved these changes

@trivikr trivikr trivikr approved these changes

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

"Authoring a large PR" contribution guide

10 participants

@mcollina @nodejs-github-bot @joyeecheung @ShogunPanda @jasnell @panva @anonrig @UlisesGascon @trivikr @RafaelGSS